-
-
Notifications
You must be signed in to change notification settings - Fork 784
New Backend - Qt #3769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Backend - Qt #3769
Conversation
|
@freakboy3742 By now I have not added CI testing yet because the test skips have not yet convereged at 100% This is rough, I need to go back and clean up comments (especially some random jokes I've made -- if you have spare time I give you permission to try to spot them) but a few things I need confirmation on:
So if beeware/briefcase#2480 (which I've filed for a feature to replace installed PySide6 with system symlink when packaging an app) doesn't get resolved in the duration of this PR, should I still remove this hack such that the new backend will not be able to look native and access system themes?
Thank you! |
|
@freakboy3742 Could you please respond to those questions I've asked about above? Thank you! |
No. Platform identification is only used because You'll also note that the current GTK backend doesn't describe its platform as GTK - it's The approach used by Textual is the model to follow here. If you're on Linux, toga_qt is a candidate backend; if that's the only backend, it's used; if there's more than one candidate installed in the current environment,
For now, I'd say yes. We can only cook with the ingredients we're given. If Qt doesn't work in virtual environments, that's a problem for Qt to resolve. It's not up to us to work around it - if only because there may be use cases for not adopting the global environment. If there's a hack that could be installed, I'd argue the
In core - No - or, at least, there's a much bigger concept that needs to be wrapped here. If the underlying question is about "system icons" - GTK has an analogous concept, which we don't currently handle. See #2441 for some initial thoughts about this (although it doesn't mention the GTK analog that exists). |
|
@freakboy3742 Thanks for the responses, there's much going on and I'm in the middle of a major comments cleanup (it's done through GitHub's review interface so expect a huge email next week with all the ntoes I made for myself, sorry about that) Re to response 1: Then how the heck am I gonna get all the tests skipped by platform? Do I need a bunch of stub probes to just say "skip on Qt", sort of like GTK4? Anyways if there's 1 platform only also for Briefcase how do we handle the testbed then? Right now I used tests_backend_qt for qt probes and hacks it at conftest.py: https://github.com/beeware/toga/pull/3769/files#diff-b845936988736dd9f884ab2aeb4175fa8ee7914217ed87d3c8aad18f803e74f0R24-R33 -- I can check the backend there [b]ut (EDITED TYPO) it just feels messy to have a hack like that at all. Re to response 2: IMO system-pyside6 for symlinking system packages is not feasible. With all this wheel madness going on you simply don't know where the final system-pyside6 package even gets installed and you can't put symlinks in wheels either (it extracts as plain text when extracted by pip, which uses its own extraction logic). There's some options for "build destination" in setup.py but that's to like an intermediate location. Should I extract this sys.modules hackery into a system-pyside6 package that installs a shim PySide6.py to replace itself, then? Re to response 3: Yeah, but it'd look really weird on Qt without like a native icon attached to the actions. KDE inserts extra space before an action with no icon to fill up for the icon space. |
|
@freakboy3742 Also do you think that beeware/briefcase#2480 is a candicate issue to be handled on Briefcase? Or are we like if PySide6 doesn't work it doesn't work and we're not going to have our packaging tool symlink a system copy? |
|
@freakboy3742 I've reworked the architechture to be much simpler to make it block the next 100ms whenever a window state change occurs (or is requested, in case there is another request between the request and the signal back from Qt). If in the 100ms block someone requests another state change from the programatic side, we buffer it and at the end of the 100ms, check for if the program requested a state change (NOT if the requested state change matches the current state), then we reapply. Multiple change events from the user in rapid succession will keep blocking until 100ms from the last one. I'll wait for test pass, and rerequest a review. All items should be resolved. EDIT -- To clarify: this will no longer handle rapid changes correctly if the user mashes keys or presses MINIMIZE in the middle of the changes, but you said in the Qt issue that this is not a major concern (as opposed to an infinite-loop architechutre). |
… that the tests are passing and I resolved the conflicts.
|
@johnzhou721 Ack - I'll take a look as soon as I can; early next week seems most likely. |
|
@freakboy3742 FYI -- While updating the support table, I also corrected some errors in it -- they should be fairly obvious. |
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting close enough that it's worth merging (if only because reviewing this PR is like swimming through molasses, and we need smaller chunks to review :-) )
I've flagged a few issues about removing the system backend for now; there's a few considerations there, so for a first pass, let's acknowledge but avoid the problem.
There's a couple of other minor docs issues that I've flagged - the biggest is to actually split the Linux docs into two pages.
docs/en/reference/platforms/linux.md
Outdated
|
|
||
| On Linux and other Unix-like operating systems, Toga currently provides 2 backends: GTK and Qt. The GTK 3 backend is mostly complete, but the Qt backend is currently a pre-alpha prototype. | ||
|
|
||
| ## GTK Backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The heading depth tweaks that have been required here leads me to think that we should probably break this into a GTK page and a Qt page, with a Linux page that provides the overall context and links to the two options.
That almost means the handful of links to linux.md that assume it's a GTK page (such as the GTK README) will also need updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freakboy3742 I've only found in the GTK README with /linux in it; let me know of other places if you find any (I used git grep on this repo.)
testbed/pyproject.toml
Outdated
| "../qt", | ||
| "PySide6-Essentials==6.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "../qt", | |
| "PySide6-Essentials==6.10.0", | |
| "../qt[pyside6]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freakboy3742 Ok... I remember the reason why I wrote it the previous way I did now that I've (mindlessly) pushed your change and the CI failed. It's because this error gets raised:
Unable to find local requirement ../qt[pyside6]
It seems that there's no handling of extras in local requirements in Briefcase's Linux integration. Is adding extras integration there mandatory for this to proceed? Or is this old way acceptable? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - this surprised me, but it's definitely happening, and I think I understand why. Logged as beeware/briefcase#2560.
In the meantime, the old approach seems the best way forward.
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
|
@freakboy3742 I've addressed all your comments, and added 4 in the inline conversations. Please review again and let me know of anything that needs changing. Thank you! |
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made one last tweak to the documentation - using an interstitial Linux landing page, rather than have completely separate documents for GTK and Qt. This is for two reasons - firstly, because users will more likely think of themselves as Linux users than GTK users; and secondly, because any existing link to reference/platforms/linux should continue to work.
But with that change, and a couple of other minor tweaks, this is definitely at the point where incremental improvement will be more helpful than trying to review this monster PR - so I think this is good to land!
Thanks for all your work on this! Now we just need to fill out all the other widgets... 🤣
testbed/pyproject.toml
Outdated
| "../qt", | ||
| "PySide6-Essentials==6.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - this surprised me, but it's definitely happening, and I think I understand why. Logged as beeware/briefcase#2560.
In the meantime, the old approach seems the best way forward.
WIP: Fixes #1142
PR Checklist: